refactor: use SpirvBuilder directly, just manage `rustc_backend_spirv` dylibs #69

Firestar99 · 2025-04-29T15:25:52Z

closes #55
closes #68
closes Rust-GPU/rust-gpu#208

The current version of cargo gpu roughly works like this, imagine it as stack frames:

cargo gpu
spawns spirv-builder-cli process
calls SpirvBuilder
spawns cargo process
spawns rustc process
loads rustc-codegen-spirv

And then we back-propagate the results:

rustc-codegen-spirv: writes spv files and json metadata of ShaderModules
Spirvbuilder: reads and parses json metadata and returns ShaderModules
spirv-builder-cli: converts rust types to version-independent ShaderModules and writes them to a json file
cargo gpu: reads json file, turns ShaderModules to Linkages (basically the same) and pretty writes a json metadata for the end user to parse

The core idea: Why not have cargo gpu directly call cargo with the required rustc-codegen-spirv flags attached, like Spirvbuilder does currently? And remove the complicated setup of spawning a new spirv-builder-cli process to call Spirvbuilder and parsing it all back out?

The key observation is that SpirvBuilder doesn't care at all what toolchain you use to build it, only it's dependency rustc_codegen_spirv has to be built by a specific toolchain. In Rust-GPU/rust-gpu#245 I make this dependency optional and hide it behind the feature rustc_codegen_spirv, which allows us to directly depend on SpirvBuilder and still build on stable. We just need to supply our SpirvBuilder with a librustc_codegen_spirv.so or rustc_codegen_spirv.dll prebuilt using the specific toolchain it requires, which significantly simplifies the build process.

This requires SpirvBuilder to stay be backwards compatible for all future versions, and we need to add support for past versions.

User-facing changes

Renames:
- capability -> capabilities
- extension -> extensions
- shader-target -> target
- debug -> release (clap: remains --debug)
- no_default_features -> default_features (clap: remains --no-default-features)
- --force-spirv-cli-rebuild -> --rebuild-codegen
- removed rust_toolchain (as you can't use a different toolchain than the one required anyway)
proper path support for rust-gpu
- will build rustc_codegen_spirv in that local path, no need to compile twice
- will always try to rebuild
faster install: removed slow rust-gpu repo cloning, instead reuse cargo's cache
faster build: fewer cmds need to be launched or things parsed
less disk space: clear target dirs of rustc_codegen_spirv after successful build

Internal changes, including to `SpirvBuilder`

removed spirv_builder_cli crate
split mod spirv_cli into spirv_source, install_toolchain and lockfile
moved most BuildArgs members to SpirvBuilder
- made all fields pub on SpirvBuilder, replacing setters (but keeping them around)
- made SpirvBuilder derive serde::Serialize, serde::Deserialize
- moved clap macro args to SpirvBuilder (masked by clap feature)
- BuildArgs has a new SpirvBuilder member with clap(flatten) and serde(flatten)
- required some renames, see above
replaced cargo tree calls with cargo_metadata crate, cherry-picked from Switch to cargo metadata #66 by @tmvkrpxl0, required to "retrieve path to manifest" during install
new install procedure:

  * check spirv-std version
- * clone rust-gpu repo at revision (slow)
- * check `toolchain.toml` for toolchain
- * copy `spirv-builder-cli` to cache dir
+ * write blank `rustc_codegen_spirv_dummy` project
+   * only depends on `rustc_codegen_spirv`, version equal to `spirv-std`
+ * check metadata for project
+   * cargo creates `Cargo.lock`
+   * cargo clones repo into it's cache, if necessary
+   * retrieve path to manifest, either in crates cache or cloned repo cache
+ * check `build.rs` of `rustc_codegen_spirv` for toolchain version
+   * has a **string literal** with the exact same contents as the `toolchain.toml`
+ * delete `Cargo.lock` to prevent lockfile version issues
  * build dummy project with determined toolchain
  * retrieve `librustc_codegen_spirv.so` or `rustc_codegen_spirv.dll`
-   * retrieve `spirv-builder-cli` binary
  * copy target-specs jsons

new build procedure:

  * cargo gpu
- * spawns `spirv-builder-cli` process
+ * set `rustc_codegen_spirv_location` and `toolchain_overwrite` on SpirvBuilder
  * calls SpirvBuilder
(...)
  * SpirvBuilder returns
- * converts `CompileResult` into `ShaderModule`s, writes to disk
- * `spirv-builder-cli` process exits
- * load `ShaderModule`s from disk
  * convert to `Linkage`s
  * write `Linkage`s to file

Todo

Firestar99 · 2025-04-29T17:23:39Z

@schell @tombh cargo gpu install works! build still needs to be adjusted

Firestar99 · 2025-04-29T17:40:46Z

I would like to see the Switch to cargo metadata PR to be merged first (or integrated into here), as you can't just use it for more efficient version querying. It also tells you where cargo has checked out the repo / downloaded the crate, which we can use to retrieve the toolchain version we need without having to clone it ourselves.

Sample:

{
  "name": "rustc_codegen_spirv",
  "version": "0.9.0",
  "id": "git+https://github.com/Rust-GPU/rust-gpu?rev=82a0f69#[email protected]",
  "license": "MIT OR Apache-2.0",
  "license_file": null,
  "description": "SPIR-V code generator backend for rustc",
  "source": "git+https://github.com/Rust-GPU/rust-gpu?rev=82a0f69#82a0f69008414f51d59184763146caa6850ac588",
  ...
  "manifest_path": "/home/firestar99/.cargo/git/checkouts/rust-gpu-d06d15e2ba0f0ae2/82a0f69/crates/rustc_codegen_spirv/Cargo.toml",
  ...
},

… cargo gpu This is bandaid fix, it now ignores RUSTC environment variable set from elsewhere, like when shader crate is part of a workspace. As of today, it fails to compile on workspace which uses nightly because an additional restriction was added to target specification json file on nightly. Even though enforcing specific rustc version seems like good idea for the purpose of cargo-gpu, target specification json file should be updated as well if we were to ever bump up required rustc version.

tombh · 2025-04-30T14:03:21Z

This looks great! So let me see if I have a rough idea of what the change is here. In short, we're replacing spirv-builder-cli with rustc_codegen_spirv_dummy (which in turns builds the custom SPIRV codegen backend for rustc)? The main benefit of which is that we can now interact directly with rust-gpu, significantly simplifying and stabilising cargo-gpu?

tombh · 2025-04-30T14:09:13Z

I think this PR would also fix this Rust-GPU/rust-gpu#208

Firestar99 · 2025-04-30T16:20:06Z

Ignore rustc_codegen_spirv_dummy that only exists to depend on rustc_codegen_spirv and have cargo build it for us. I'll try to write a better summary in the description of all the changes.

tombh

Everything looks great. Just a couple of minor suggestions.

crates/cargo-gpu/src/args.rs

crates/cargo-gpu/src/linkage.rs

crates/cargo-gpu/src/metadata.rs

crates/cargo-gpu/src/lockfile.rs

Firestar99 · 2025-05-06T16:23:30Z

@tombh feel free to rereview and merge, but I got one followup question:

cargo-gpu/crates/xtask/src/main.rs

Lines 154 to 188 in 6408188

    
           log::info!("installing cargo gpu artifacts"); 
        
           cmd([ 
        
               "cargo", 
        
               "gpu", 
        
               "install", 
        
               "--shader-crate", 
        
               SHADER_CRATE_PATH, 
        
               "--auto-install-rust-toolchain", 
        
               "--force-overwrite-lockfiles-v4-to-v3", 
        
           ]) 
        
           .unwrap(); 
        
           let dir = tempdir::TempDir::new("test-shader-output").unwrap(); 
        
           let mut overwriter = ShaderCrateTemplateCargoTomlWriter::new(); 
        
           overwriter.replace_output_dir(dir.path()).unwrap(); 
        
           if let Some(rust_gpu_version) = maybe_rust_gpu_version { 
        
               if rust_gpu_version != "latest" { 
        
                   overwriter 
        
                       .replace_spirv_std_version(rust_gpu_version) 
        
                       .unwrap(); 
        
               } 
        
           } 
        
           cmd([ 
        
               "cargo", 
        
               "gpu", 
        
               "build", 
        
               "--shader-crate", 
        
               SHADER_CRATE_PATH, 
        
               "--auto-install-rust-toolchain", 
        
               "--force-spirv-cli-rebuild", 
        
               "--force-overwrite-lockfiles-v4-to-v3", 
        
           ]) 
        
           .unwrap();

Why are we installing one rust-gpu version (whatever is in the repo, currently a rev), then replace the version with what was specified in the cmdline and when building, reinstall a different rust-gpu version? I was wondering why our 0.9.0 and 0.8.0 builds were taking twice as much as the latest does. Any specific reason it has been done like that, or should we unify to testing only one version?

tombh · 2025-05-07T00:14:12Z

Wow, that doesn't seem right. I can't think of any reason why that might be. I wonder if @schell does? I think as long as the tests pass then just a single cargo build ... should be enough?

tmvkrpxl0 · 2025-05-07T00:30:24Z

I like how we are discovering what it does along the way

schell · 2025-05-10T21:27:05Z

No clue why it does that, but it's probably just cruft from having us all hack on it with abandon, as they say :)

schell · 2025-05-10T21:29:57Z

README.md

+
+          Usage: cargo-gpu show capabilities
+
+          Options:


I wish there were a way we could keep the usage in the README in sync with the tool without having to manually insert it here. 🤔

Thank you for doing it, though, obviously.

crates/cargo-gpu/src/args.rs

schell

Great work! I checked out the source and this compiles my project.

I only had some quick nitpicks about typos. Can't wait to see this merge.

Firestar99 · 2025-05-10T22:10:26Z

The version 0.8.0 and 0.9.0 builds are now just as fast as latest on ubuntu (not to speak of windows taking twice as long...)

Firestar99 · 2025-05-12T08:27:15Z

@tombh as you requested changes earlier, github wants your approval before we can merge

… 200MiB

Firestar99 · 2025-05-12T09:13:51Z

Added a few more things:

remove the target dir of rustc_codegen_spirv build, about 200MiB of saved disk space
rename --force-spirv-cli-rebuild to --rebuild-codegen and fixed other docs mentioning it

tombh · 2025-05-12T13:04:46Z

Apologies, was under the impression that we only needed one approval to merge. Noted.

I'll take the opportunity again to say how great this PR is. Thank you.

Firestar99 · 2025-05-12T20:28:53Z

I can only squash and merge. Could we switch it to just merging (no squashing) for just this PR, as it does change a lot?

tombh · 2025-05-12T21:00:42Z

I don't appear to have permissions for that, I'm sure @schell does.

Just to throw in my 2 cents, I'm not totally convinced that these commits would be better merged as they are over being squashed. Whilst they do tell an important story, they also clutter up main, making reading said story harder in other contexts, say for bisecting a bug. But don't let that stop you merging! Just wanted to mention it.

Firestar99 added 3 commits April 29, 2025 17:33

spirv_source minor cleanup

cb1d8b1

gitignore: .idea

ca952e1

cargo gpu: remove ignored profiles emitting warnings

32c6d62

Firestar99 force-pushed the total_refactor branch from ba36f53 to 32c6d62 Compare April 29, 2025 15:33

Firestar99 added 7 commits April 29, 2025 17:34

remove spirv-builder-cli, leave install broken

21bddf5

minor cleanup

4d140a0

adjust install

859af40

fix clap and serde errors

a524212

working install, build broken

897531a

adjust tests to changed names

7f6552f

some clippy fixes

f5660df

Firestar99 mentioned this pull request Apr 29, 2025

Switch to cargo metadata #66

Open

tmvkrpxl0 and others added 9 commits April 30, 2025 00:44

replace cargo tree with cargo metadata

021073e

fix local path spirv-std dependency

4f5b9cb

removed repo cloning, grab toolchain from cargo's checkout

eacfbc8

reactive slow test, no longer clones a repo

8df2452

port mod metadata to cargo_metadata

2a74f92

split apart spirv_cli into spirv_source, install_toolchain and lockfile

dda18d9

support for local paths, also always build it there instead of our cache

6fef1fd

support for old rust-gpu versions on crates.io

8b5ea07

Firestar99 force-pushed the total_refactor branch from b826762 to f02f61c Compare April 30, 2025 13:28

Firestar99 added 3 commits April 30, 2025 19:50

adjust build action to use SpirvBuilder

37512d9

don't remove Cargo.lock for local paths

60fb2e5

use target-specs from spirv-builder

595dd1b

Firestar99 requested review from tombh and schell May 1, 2025 13:47

tombh requested changes May 2, 2025

View reviewed changes

Firestar99 added 4 commits May 6, 2025 12:42

use master rust-gpu

82468e4

apply suggestions from @tombh

2c333e7

rename mentions of --capability to --capabilities

b06ee19

update readme's dump-usage

363b6ec

Firestar99 marked this pull request as ready for review May 6, 2025 10:55

schell reviewed May 10, 2025

View reviewed changes

crates/cargo-gpu/src/args.rs Outdated Show resolved Hide resolved

schell approved these changes May 10, 2025

View reviewed changes

Firestar99 added 2 commits May 10, 2025 23:58

fix typo

7f3a9d8

ci: speed up old version builds by not building current version

f62023f

Firestar99 added 3 commits May 12, 2025 11:12

update main docs

3935805

remove target dir after successful rustc_codegen_spirv build, saves…

eef51fe

… 200MiB

rename --force-spirv-cli-rebuild to --rebuild-codegen

a048eb5

Firestar99 force-pushed the total_refactor branch from d074885 to a048eb5 Compare May 12, 2025 09:13

Firestar99 mentioned this pull request May 12, 2025

Make cargo-gpu a library, usable from build scripts #71

Draft

tombh approved these changes May 12, 2025

View reviewed changes

Firestar99 merged commit 2180d73 into Rust-GPU:main May 12, 2025
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use SpirvBuilder directly, just manage `rustc_backend_spirv` dylibs #69

refactor: use SpirvBuilder directly, just manage `rustc_backend_spirv` dylibs #69

Firestar99 commented Apr 29, 2025 •

edited

Loading

Firestar99 commented Apr 29, 2025

Firestar99 commented Apr 29, 2025

tombh commented Apr 30, 2025

tombh commented Apr 30, 2025

Firestar99 commented Apr 30, 2025

tombh left a comment

Firestar99 commented May 6, 2025 •

edited

Loading

tombh commented May 7, 2025

tmvkrpxl0 commented May 7, 2025

schell commented May 10, 2025

schell May 10, 2025

schell May 10, 2025

schell left a comment

Firestar99 commented May 10, 2025

Firestar99 commented May 12, 2025

Firestar99 commented May 12, 2025

tombh commented May 12, 2025

Firestar99 commented May 12, 2025

tombh commented May 12, 2025

refactor: use SpirvBuilder directly, just manage rustc_backend_spirv dylibs #69

refactor: use SpirvBuilder directly, just manage rustc_backend_spirv dylibs #69

Conversation

Firestar99 commented Apr 29, 2025 • edited Loading

User-facing changes

Internal changes, including to SpirvBuilder

Todo

Firestar99 commented Apr 29, 2025

Firestar99 commented Apr 29, 2025

tombh commented Apr 30, 2025

tombh commented Apr 30, 2025

Firestar99 commented Apr 30, 2025

tombh left a comment

Choose a reason for hiding this comment

Firestar99 commented May 6, 2025 • edited Loading

tombh commented May 7, 2025

tmvkrpxl0 commented May 7, 2025

schell commented May 10, 2025

schell May 10, 2025

Choose a reason for hiding this comment

schell May 10, 2025

Choose a reason for hiding this comment

schell left a comment

Choose a reason for hiding this comment

Firestar99 commented May 10, 2025

Firestar99 commented May 12, 2025

Firestar99 commented May 12, 2025

tombh commented May 12, 2025

Firestar99 commented May 12, 2025

tombh commented May 12, 2025

refactor: use SpirvBuilder directly, just manage `rustc_backend_spirv` dylibs #69

refactor: use SpirvBuilder directly, just manage `rustc_backend_spirv` dylibs #69

Firestar99 commented Apr 29, 2025 •

edited

Loading

Internal changes, including to `SpirvBuilder`

Firestar99 commented May 6, 2025 •

edited

Loading